-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow GesturePlatformManager to be created for views with null window… #18938
Allow GesturePlatformManager to be created for views with null window… #18938
Conversation
Hey there @artemvalieiev! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
@dotnet-policy-service agree |
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested through this a bit, and it looks like it works.
I'd like to figure out a more overall fix that we can apply where we are able to actually set a window of some sort for embedded scenarios.
There are a few cases where that's going to be relevant in order to fix this overall. For example, loaded/unloaded events are also broken currently on embedded.
If we still don't have a great fix in the SR2 timeline, then I think it'd be worth merging this and possibly doing something similar for loaded/unloaded.
We have this issue: #1718 which tracks the concept a bit. I think we can revisit the bigger picture in an upcoming sprint soon to see what changes we would need to make and scope the effort for doing this the 'right way'. |
src/Controls/src/Core/Platform/GestureManager/GestureManager.cs
Outdated
Show resolved
Hide resolved
src/Controls/src/Core/Platform/GestureManager/GestureManager.cs
Outdated
Show resolved
Hide resolved
src/Controls/tests/Core.UnitTests/Gestures/GestureManagerTests.cs
Outdated
Show resolved
Hide resolved
d94f6db
to
5100edb
Compare
5100edb
to
1818379
Compare
Added this to the larger effort for next year to make embedding work better in all cases: #1718 |
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@artemvalieiev thank you for this PR!
This is a great way for us to resolve this issue until we can build a better embedding story.
From what I can tell, we only regressed gestures between net7/net8. Other areas, like unloaded and loaded, were unfortunately never working.
We are actively working on the overall story here. These changes make sense until we can provide a better overall fix.
Description of Change
In the case of Maui Embedding, there is no window, and the gesture manager is always skipping the initialization of PlatformManager.
My changes allow a GestureManager to be created with a null window. I also keep the behavior of recreating/clearing the GesturePlatformManager when the window is removed or replaced.
Issues Fixed
Fixes [regression/8.0.0] TapGestureRecognizers not working in maui embedded